-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extensive changes #14
base: master
Are you sure you want to change the base?
Conversation
I have created the dev-GCM-pull14 branch in order to test the changes. I will make some minor changes, in order to preserve backward compatibility. Then I will commit in order to test it, and then I can merge it to master. |
Hi @agimenezwally, I have just realized that I did not mentioned you in the previous comment. Did you or @gusmanb had a chance to test 24445c7 for GCM? I would like to merge it with the main branch, but before that I would like you to confirm that it is working for your use cases. |
HI @pgstath . Sorry for the delay, I'm on a business travel so I can't test it until next week... Cheers. |
Hi @pgstath sorry for the delay I've been really busy these months. Now I have some spare time, if you guide me a bit on how can I retrieve all the changes and test it I can check this week if everything works with GCM (I'm not very good with branches, pull requests, versions and so on on GIT, sorry :( ) |
HI @olibos Of course, if it's working I would be more than glad to see this merged on main. Also @pgstath if you want to remove the bodyless message event and mix it with the message event, it would be great, just need to remove the event and were was the rising change it to this code:
Sorry for the formatting, I have no clue on how to add multiline code... |
Hello, I have made the change from @gusmanb in my forked repo: https://github.com/olibos/Sharp.Xmpp/tree/dev-GCM-pull14 Just a quick question, is there any reason why XmppExtension, IInputFilter and IOutputFilter are internals? If everything related to Extensions are switched to public it may allow to extend XMPP with some proprietary handling.
If it's public we can avoid adding proprietary code inside the core library and add those inside another library which use Sharp.Xmpp and then avoid to reinvent the wheel in each FCM client application. |
I can see two things here: 1-Control messages must be managed by the app or library, I can't see any benefit of using an extension, per example in my implementation when an ACK is received I remove the messages from a retry queue, so an extension is not viable for it, and for the draining, when I receive it I create a new connection and move the draining connection to a separate management queue to destroy it when the draining is finished. 2-Despite point 1, it may be useful in other cases to have the functionality of extensions available, maybe transformation of an incomming/outgoing message to simplify main code, or to support different data codification per example. So if @pgstath agrees I belive it's a good and viable enhancement. |
Hi @gusmanb , I agree, but the control message could be managed in an event inside the extension like UserAvatar.AvatarChanged event. I will probably make a proof of concept if @pgstath makes extensions public ;-) |
Hmmm, good point @olibos and nice thought, this can really simplify the code and make it more maintainable. Cheers. |
Hi guys, apologies for the delay, some quite heavy workload during this period. I will try to follow up with the discussion and test the pull request. |
Hi guys. I'm just here to inform I have ported the library to .net Core, I had to replace the dns resolution system, make some things async and blablabla :D If you want to take a look at it it's here: https://github.com/gusmanb/Sharp.Xmpp.NetCore It have mixed the most interesting changes I have seen (olivo's BodylessMessage unification, UseRooster changed to RetrieveRooster and all my previous changes) but if I missed something don't be hesitate to do a pull request. This is still under test, but the base to make it work is here, I'm going to work on this for the next weeks until I got all running and stable as I want to change all the API to be async. Cheers. |
from an XmlElement (h/t pgstath#14)
It would be great if this issue could be resolved and merged into mainline. Trying to connect to FCM via Sharp.Xmpp at the moment is kind of a pain since the NuGet version does not work. Is anyone following this up? |
from an XmlElement (h/t pgstath#14)
Current changes:
1-Added a new constructor to IM.Message which allows to create a message from an XmlElement
2-Left the Xmpp.Xml namespace public to allow to use the functions to create/append Xml elements
3-Implemented a new event named BodylessMessage which receives messages with no body
4-BodylessMessage uses MessageEventArgs, for that I created a new constructor which does not require a Jid as these messages come without an ID (they come from the server itself, not from an user)
6-Created a property on the connection to disable roster retrieval when connection is created.
7-Created a public function to send a default Presence stanza as the usual with no roster is to not send the presence, but in case some service uses it without roster the function is available.
8-Changed Tls from bool to enum, this allows to use StartTLS extension or Ssl sockets.
9-Updated ArSoft.Tools.Net reference